fix(security): close CodeQL #33 go/command-injection in runGit (Critical)#55
Merged
fix(security): close CodeQL #33 go/command-injection in runGit (Critical)#55
Conversation
CodeQL alert #33 (Critical): exec.Command("git", ...) in runGit received user-controlled strings (commit message bytes from author / subject, and paths from note keys). Although exec.Command uses argv (no shell) which makes the standard shell-injection vector inert, git itself has flag-based attack surface (e.g. `--upload-pack=cmd`, `-c core.sshCommand=...`) that CodeQL is right to flag. Defense in depth: 1. `runGit` now enforces: - notesDir must be non-empty and must not start with "-" (prevents it being parsed as a git top-level flag). - args[0] must be in a closed allow-list of subcommands (init / config / add / log). Commit is deliberately routed elsewhere (see #2). - Any arg after a literal "--" must satisfy filepath.IsLocal — this continues the sanitiser from PR #44 and is what CodeQL recognises. 2. `gitCommit` is a new helper that runs `git commit --no-gpg-sign -F -` and pipes the message via stdin. The message bytes therefore never appear in argv, cutting the only remaining taint flow from user input to exec.Command args. 3. autoCommit's commit call switched from runGit(..., "commit", "-m", msg) to gitCommit(notesDir, msg). Tested locally: 104/104 tests passing in internal/notes/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alert
CodeQL alert #33 — `go/command-injection` — Critical severity, `internal/notes/history.go:106`.
`exec.Command("git", ...)` in `runGit` received user-controlled strings: commit message bytes (author + subject), and relative paths derived from note keys. While `exec.Command` uses argv (no shell) which disables the classic shell-injection vector, git itself has flag-based attack surface that CodeQL is right to flag:
Fix — defense in depth
1. `runGit` gains a strict sanitiser pass:
2. `commit` is deliberately removed from the allow-list. Commit messages come from user input; routing them through argv was the last remaining taint flow. A new `gitCommit(notesDir, msg)` helper runs:
```
git -C commit --no-gpg-sign --allow-empty-message -F -
```
and pipes the message via stdin. User bytes never appear in argv.
3. `autoCommit` switched from `runGit(..., "commit", "-m", msg)` to `gitCommit(notesDir, msg)`.
Why this clears the alert
CodeQL's taint-tracking for `go/command-injection`:
Test results
Local: `104/104 tests passing in internal/notes/`, `go vet` clean.
Files changed
Not addressed
Alert #11 — SAST score 9/10 (27/30 commits scanned). Cosmetic Scorecard warning. CodeQL runs on PR + push-to-main + weekly schedule — intermediate feature-branch commits aren't individually scanned. Fixing to 30/30 would require running CodeQL on every push to every branch, which is expensive for a pre-1.0 single-maintainer project. Leaving as-is.
🤖 Generated with Claude Code